Reviewed 7 of 14 files at r1, 2 of 3 files at r3, all commit messages.
Reviewable status: 9 of 14 files reviewed, 2 unresolved discussions (waiting on @Jontified)

mullvad-paths/src/windows.rs line 71 at r3 (raw file):

/// only directories that do not already exist and the leaf directory will have their permissions set.
pub fn create_dir_recursive(path: &Path, set_security_permissions: bool) -> Result<()> {
    if set_security_permissions {

Are the branches flipped? set_security_permissions == true seems to imply that permissions should be set.

windows/windows-libraries line 1 at r3 (raw file):

Subproject commit 6df03881c5bbde740ba601906594be61b99319a7

In my opinion, wrapping create_privileged_directory in libcommon creates a strange dependency for libcommon on the app repository. That's supposed to be an independent library. Moreover, the point of mullvad-nsis it to depend less on it, if anything.

I suggest either (a) use create_privileged_directory directly in the projects, or (b) put the wrapper in libshared (not a submodule).

Reviewable status: 9 of 14 files reviewed, 3 unresolved discussions (waiting on @Jontified)

mullvad-paths/src/windows.rs line 146 at r3 (raw file):

    // Authenticated users SID https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers
    let (authenticated_users_ea, authenticated_users_psid) =
        match create_explicit_access("S-1-5-11", GENERIC_READ) {

This is fine, but it seems like we could have used CreateWellKnownSid(WinAuthenticatedUserSid, ...) and CreateWellKnownSid(WinBuiltinAdministratorsSid, ...) here.

A nice thing about it is that we allocate the memory ourselves, so most of the LocalFrees go away, and we only have to use plain arrays or Vecs. Cleaner, probably?

Though I vaguely recall that this did not work for some reason.

mullvad-paths/src/windows.rs line 217 at r3 (raw file):

    let sid_wide_str = get_wide_str(sid_wide_str);
    let mut psid = ptr::null_mut();
    if 0 == unsafe { ConvertStringSidToSidW(sid_wide_str.as_ptr(), &mut psid) } {

I think we should avoid Yoda conditions, as they're inconsistent with our general code style. The point of these is mainly to prevent accidental assignment, which isn't relevant for if statements in Rust.

Reviewable status: 8 of 14 files reviewed, 4 unresolved discussions (waiting on @dlon and @faern)

mullvad-paths/src/windows.rs line 68 at r3 (raw file):

Done.

mullvad-paths/src/windows.rs line 71 at r3 (raw file):

Done.

mullvad-paths/src/windows.rs line 217 at r3 (raw file):

Done.

windows/windows-libraries line 1 at r3 (raw file):

Done. Since it's only called in log I think it's cleaner to just have the call there.

Reviewed 2 of 14 files at r1, 1 of 1 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faern and @Jontified)

mullvad-nsis/Cargo.toml line 18 at r6 (raw file):

[target.i686-pc-windows-msvc.build-dependencies]
cbindgen = { version = "0.24.3", default-features = false }

Nit: Diff just removes a newline.

mullvad-paths/src/windows.rs line 245 at r6 (raw file):

    unsafe { LocalFree(new_dacl as isize) };

    if ERROR_SUCCESS != result {

Still one over here, but it's not a big deal.

windows/nsis-plugins/src/log/log.cpp line 301 at r6 (raw file):

        if (Status::Ok != create_privileged_directory(reinterpret_cast<const uint16_t*>(w_path)))
        {
            THROW_ERROR("Failed to create privileged directory");

Nit: Fine, but why a needlessly vague "privileged directory" rather than "log directory"?

windows/nsis-plugins/src/log/log.vcxproj line 103 at r6 (raw file):

      <ModuleDefinitionFile>log.def</ModuleDefinitionFile>
    </Link>
  </ItemDefinitionGroup>

Looks like this file is missing a pre build event (see cleanup.vcxproj).

windows/nsis-plugins/src/tray/tray.vcxproj line 72 at r6 (raw file):

      <GenerateDebugInformation>true</GenerateDebugInformation>
      <ImageHasSafeExceptionHandlers>false</ImageHasSafeExceptionHandlers>
      <AdditionalLibraryDirectories>$(ProjectDir)../../../../target/i686-pc-windows-msvc/release;$(ProjectDir)../../../../dist-assets/binaries/x86_64-pc-windows-msvc/nsis/;$(SolutionDir)bin\$(Platform)-$(Configuration)\</AdditionalLibraryDirectories>

Can the changes be reverted for this project?

windows/windows-libraries line 1 at r3 (raw file):

Awesome!

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @faern and @Jontified)

windows/nsis-plugins/src/log/log.cpp line 297 at r6 (raw file):

		logpath.append(L"Mullvad VPN");

        const wchar_t* w_path = logpath.wstring().c_str();

Should be tabs.

Reviewable status: 9 of 14 files reviewed, 3 unresolved discussions (waiting on @dlon and @faern)

windows/nsis-plugins/src/log/log.cpp line 297 at r6 (raw file):

Done.

windows/nsis-plugins/src/log/log.vcxproj line 103 at r6 (raw file):

Done.

Reviewed 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Jontified)

windows/nsis-plugins/src/log/log.cpp line 297 at r7 (raw file):

		logpath.append(L"Mullvad VPN");

		const wchar_t* w_path = logpath.wstring().c_str();

Use after free -- the std::wstring is destroyed here.

Reviewable status: 13 of 14 files reviewed, 1 unresolved discussion (waiting on @dlon)

windows/nsis-plugins/src/log/log.cpp line 297 at r7 (raw file):

Good catch, done.

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved